[Rebase & FF] Make PageTableHal stateful and eliminate global statics from test code - #222
Open
vineelko wants to merge 2 commits into
Open
[Rebase & FF] Make PageTableHal stateful and eliminate global statics from test code#222vineelko wants to merge 2 commits into
vineelko wants to merge 2 commits into
Conversation
os-d
reviewed
Aug 17, 2026
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
makubacki
reviewed
Aug 20, 2026
| @@ -241,6 +246,7 @@ impl<P: PageAllocator, Arch: PageTableHal> PageTableInternal<P, Arch> { | |||
|
|
|||
| fn map_memory_region_internal( | |||
Collaborator
There was a problem hiding this comment.
The number of parameters is starting to reach the point that it's probably worth considering a context struct where it makes sense.
Contributor
Author
There was a problem hiding this comment.
Every api is slightly different we probably need different context structures. I will try to take it as a separate PR.
Collaborator
There was a problem hiding this comment.
Yeah, this was one of the more extreme cases I found.
vineelko
force-pushed
the
users/vineelko/remove_global_statics_0817
branch
2 times, most recently
from
August 21, 2026 00:10
69842c2 to
ea29786
Compare
By declaring only associated functions in the PageTableHal trait, it becomes impossible to cleanly define any kind of state that can be controlled for testing purposes. Also, the current usage of these associated functions is not truly stateless. Instead, their state is hidden behind the hardware registers. This may be fine for the non test code, where the generic Arch type parameter bounded by PageTableHal is passed all the way from PageTable to PageTableHal implicitly. But its usage complicates testing by forcing hacky global statics, and on top of that, to keep those statics from pounding on each other forces to serialize the tests with `serial_test`. This commit converts every PageTableHal method from an associated function to a &self method so tests can carry per instance state instead of relying on globals. The arch instance is now plumbed explicitly through the paging core rather than being invoked purely by generic type. No functional change to mapping/query/unmap behavior. Signed-off-by: Vineel Kovvuri[MSFT] <vineelko@microsoft.com>
Replace the global ACTIVE/BASE statics in the paging unit tests with per instance AtomicBool/AtomicU64 fields on DummyArch, shared with DummyAllocator via Rc<DummyArch>. Each test now owns its own arch state instead of mutating process wide globals, so the tests no longer need #[serial] and can run in parallel. This also makes the tests more useful under nextest. Signed-off-by: Vineel Kovvuri[MSFT] <vineelko@microsoft.com>
vineelko
force-pushed
the
users/vineelko/remove_global_statics_0817
branch
from
August 21, 2026 23:50
ea29786 to
2e99976
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Make PageTableHal a stateful (&self) trait
By declaring only associated functions in the PageTableHal trait, it
becomes impossible to cleanly define any kind of state that can be
controlled for testing purposes. Also, the current usage of these
associated functions is not truly stateless. Instead, their state is
hidden behind the hardware registers. This may be fine for the non test
code, where the generic Arch type parameter bounded by PageTableHal is
passed all the way from PageTable to PageTableHal implicitly. But its
usage complicates testing by forcing hacky global statics, and on top
of that, to keep those statics from pounding on each other forces to
serialize the tests with
serial_test.This commit converts every PageTableHal method from an associated
function to a &self method so tests can carry per instance state
instead of relying on globals. The arch instance is now plumbed
explicitly through the paging core rather than being invoked purely by
generic type.
No functional change to mapping/query/unmap behavior.
Make DummyArch stateful to drop serial_test
Replace the global ACTIVE/BASE statics in the paging unit tests with per
instance AtomicBool/AtomicU64 fields on DummyArch, shared with
DummyAllocator via Rc. Each test now owns its own arch state
instead of mutating process wide globals, so the tests no longer
need #[serial] and can run in parallel.
This also makes the tests more useful under nextest.
How This Was Tested
Booted in Q35, Ran repeated rounds of tests both under
cargo make testandcargo nextest runIntegration Instructions
NA